Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

completely replace underscore with lodash #315

Closed
wants to merge 1 commit into from

Conversation

kessler
Copy link

@kessler kessler commented May 4, 2014

lodash is a suprioer superset of underscore (full api compatibility on stable versions) and is far superior in many ways.

…set of underscore (full api compatibility on stable versions)
@jamlen
Copy link
Contributor

jamlen commented May 4, 2014

👍 from me... I always use lodash over underscore now!

@mgan59
Copy link
Contributor

mgan59 commented May 5, 2014

👍

@moll
Copy link
Contributor

moll commented May 5, 2014

Please be aware that there are few differences between them, first and biggest of which is their behavior when it comes to inherited properties of objects given to _.extend. Underscore loops over all, incl. inherited properties, while Lodash doesn't.

@JedWatson
Copy link
Member

I've been sticking to underscore over lodash because they're more strict about only including features that can't be misinterpreted (e.g. the limitations on _.extend) as a language addition, whereas lodash has more features but seems more likely to do something unexpected or non-standard.

It's a bit hard to explain what I mean by that - generally underscore seems more inline with how features (_.each, _.map, etc) are eventually implemented in Javascript.

I'm happy to go with community consensus on this and switch. @kessler could you be a bit more specific though with your "far superior in many ways" comment?

The speed improvement seems like the biggest argument (given that common niceties we need are also added to the keystone-utils library as required). I've cherry-picked the benchmarks that are relevant based on function I know keystone uses often, and excluding ones too close to call:

_(...) with a number:
lodash.min x 35,584,905 ops/sec ±3.98% (58 runs sampled)
underscore-min x 21,685,676 ops/sec ±4.62% (53 runs sampled)
lodash.min is 65% faster.

_(...) with an array:
lodash.min x 23,199,214 ops/sec ±1.46% (60 runs sampled)
underscore-min x 26,382,733 ops/sec ±1.35% (59 runs sampled)
underscore-min is 14% faster.

_(...) with an object:
lodash.min x 13,812,252 ops/sec ±2.40% (57 runs sampled)
underscore-min x 27,753,155 ops/sec ±1.27% (58 runs sampled)
underscore-min is 103% faster.

_.bind:
lodash.min x 1,838,004 ops/sec ±2.50% (57 runs sampled)
underscore-min x 1,501,553 ops/sec ±2.04% (57 runs sampled)
lodash.min is 22% faster.

bound call with arguments:
lodash.min x 4,325,429 ops/sec ±1.66% (59 runs sampled)
underscore-min x 7,554,872 ops/sec ±2.13% (56 runs sampled)
underscore-min is 74% faster.

bound and partially applied call with arguments:
lodash.min x 3,956,844 ops/sec ±2.13% (58 runs sampled)
underscore-min x 6,926,837 ops/sec ±3.94% (55 runs sampled)
underscore-min is 72% faster.

_.clone with an object:
lodash.min x 359,662 ops/sec ±1.56% (59 runs sampled)
underscore-min x 522,627 ops/sec ±2.14% (55 runs sampled)
underscore-min is 44% faster.

_.compact:
lodash.min x 3,935,837 ops/sec ±1.57% (58 runs sampled)
underscore-min x 1,196,394 ops/sec ±1.15% (60 runs sampled)
lodash.min is 228% faster.

_.contains iterating an array:
lodash.min x 10,701,042 ops/sec ±1.48% (62 runs sampled)
underscore-min x 2,937,160 ops/sec ±3.31% (61 runs sampled)
lodash.min is 271% faster.

_.contains iterating an object:
lodash.min x 566,682 ops/sec ±1.61% (58 runs sampled)
underscore-min x 412,175 ops/sec ±2.82% (55 runs sampled)
lodash.min is 39% faster.

_.defaults:
lodash.min x 291,941 ops/sec ±2.17% (59 runs sampled)
underscore-min x 327,058 ops/sec ±4.09% (56 runs sampled)
underscore-min is 10% faster.

_.each iterating an array:
lodash.min x 1,680,530 ops/sec ±3.36% (43 runs sampled)
underscore-min x 1,211,599 ops/sec ±2.13% (60 runs sampled)
lodash.min is 37% faster.

_.each iterating an array with thisArg (slow path):
lodash.min x 230,889 ops/sec ±2.09% (55 runs sampled)
underscore-min x 259,713 ops/sec ±2.65% (57 runs sampled)
underscore-min is 12% faster.

_.each iterating an object:
lodash.min x 462,851 ops/sec ±2.16% (58 runs sampled)
underscore-min x 402,994 ops/sec ±1.73% (59 runs sampled)
lodash.min is 14% faster.

_.extend:
lodash.min x 391,455 ops/sec ±1.74% (60 runs sampled)
underscore-min x 479,454 ops/sec ±4.37% (53 runs sampled)
underscore-min is 19% faster.

_.filter iterating an array:
lodash.min x 1,723,711 ops/sec ±1.61% (56 runs sampled)
underscore-min x 1,129,047 ops/sec ±3.54% (57 runs sampled)
lodash.min is 56% faster.

_.filter iterating an array with thisArg (slow path):
lodash.min x 230,036 ops/sec ±1.94% (59 runs sampled)
underscore-min x 239,342 ops/sec ±2.60% (58 runs sampled)
underscore-min is 3% faster.

_.filter iterating an object:
lodash.min x 365,785 ops/sec ±3.47% (51 runs sampled)
underscore-min x 324,787 ops/sec ±1.42% (58 runs sampled)
lodash.min is 10% faster.

_.map iterating an array:
lodash.min x 1,719,196 ops/sec ±2.07% (44 runs sampled)
underscore-min x 1,204,887 ops/sec ±3.80% (60 runs sampled)
lodash.min is 45% faster.

_.map with thisArg iterating an array (slow path):
lodash.min x 203,691 ops/sec ±4.32% (55 runs sampled)
underscore-min x 254,401 ops/sec ±2.55% (59 runs sampled)
underscore-min is 27% faster.

_.map iterating an object:
lodash.min x 408,423 ops/sec ±4.37% (55 runs sampled)
underscore-min x 336,210 ops/sec ±2.47% (58 runs sampled)
lodash.min is 19% faster.

_.pluck:
lodash.min x 1,275,845 ops/sec ±4.73% (52 runs sampled)
underscore-min x 858,254 ops/sec ±1.59% (59 runs sampled)
lodash.min is 44% faster.

_.template (slow path):
lodash.min x 12,955 ops/sec ±3.12% (52 runs sampled)
underscore-min x 22,679 ops/sec ±2.75% (54 runs sampled)
underscore-min is 76% faster.

Seems like the results lean slightly in favour of lodash, not the comprehensive 1.4x improvement when you look at the overall benchmark results though (disclaimer: I didn't use a calculator)

Are there particular lodash functions that anyone has been looking to use in keystone development that would make a definitive argument to switch?

Finally, it may also be worth switching to use of native methods as a rule for functionality available in the V8 implementation of Javascript (e.g. Array.prototype.filter rather than _.filter) for the biggest speed improvement; to date I've generally been using _ so there's less context switch between writing client and server side code.

@moll
Copy link
Contributor

moll commented May 5, 2014

I've been sticking to underscore over lodash because they're more strict about only including features that can't be misinterpreted (e.g. the limitations on _.extend) as a language addition, whereas lodash has more features but seems more likely to do something unexpected or non-standard.

I'm happy to go with community consensus on this and switch. @kessler could you be a bit more
specific though with your "far superior in many ways" comment?

@JedWatson, I've got your back! Screw community consensus. Doing the simpler thing is more important!

@kessler
Copy link
Author

kessler commented May 5, 2014

Hi, Give me several days, I will conduct a more thorough "survey" and will come back with specifics.

@kessler
Copy link
Author

kessler commented May 7, 2014

Firstly, I ran all these benchmarks from http://lodash.com/benchmarks using Lo-Dash(modern) and Underscore (production), on my machine (i3 4gigs mem, windows 7, chrome 34) and came back with extremely different results. I never expected such a huge difference, its troubling. Here is a side by side comparison:

_(...) with a number:
lodash.min x 35,584,905 ops/sec ±3.98% (58 runs sampled)
underscore-min x 21,685,676 ops/sec ±4.62% (53 runs sampled)
lodash.min is 65% faster.

_(...) with a number:
lodash.min x 32,953,052 ops/sec ±8.89% (58 runs sampled)
underscore-min x 10,698,620 ops/sec ±2.14% (76 runs sampled)
lodash.min is 189% faster.

_(...) with an array:
lodash.min x 23,199,214 ops/sec ±1.46% (60 runs sampled)
underscore-min x 26,382,733 ops/sec ±1.35% (59 runs sampled)
underscore-min is 14% faster.

_(...) with an array:
lodash.min x 35,363,311 ops/sec ±3.80% (57 runs sampled)
underscore-min x 11,164,659 ops/sec ±0.79% (89 runs sampled)
lodash.min is 208% faster.

_(...) with an object:
lodash.min x 13,812,252 ops/sec ±2.40% (57 runs sampled)
underscore-min x 27,753,155 ops/sec ±1.27% (58 runs sampled)
underscore-min is 103% faster.

_(...) with an object:
lodash.min x 7,138,055 ops/sec ±1.18% (86 runs sampled)
underscore-min x 11,336,961 ops/sec ±0.90% (85 runs sampled)
underscore-min is 59% faster.

_.bind:

lodash.min x 1,838,004 ops/sec ±2.50% (57 runs sampled)
underscore-min x 1,501,553 ops/sec ±2.04% (57 runs sampled)
lodash.min is 22% faster.

lodash.min x 310,267 ops/sec ±8.47% (38 runs sampled)
underscore-min x 282,962 ops/sec ±4.52% (76 runs sampled)
lodash.min is 6% faster.

bound call with arguments:
lodash.min x 4,325,429 ops/sec ±1.66% (59 runs sampled)
underscore-min x 7,554,872 ops/sec ±2.13% (56 runs sampled)
underscore-min is 74% faster.

bound call with arguments:
lodash.min x 4,875,489 ops/sec ±0.92% (87 runs sampled)
underscore-min x 2,567,662 ops/sec ±2.95% (81 runs sampled)
lodash.min is 94% faster.

bound and partially applied call with arguments:
lodash.min x 3,956,844 ops/sec ±2.13% (58 runs sampled)
underscore-min x 6,926,837 ops/sec ±3.94% (55 runs sampled)
underscore-min is 72% faster.

bound and partially applied call with arguments:
lodash.min x 2,176,781 ops/sec ±1.45% (85 runs sampled)
underscore-min x 1,999,986 ops/sec ±3.40% (79 runs sampled)
lodash.min is 11% faster.

_.clone with an object:
lodash.min x 359,662 ops/sec ±1.56% (59 runs sampled)
underscore-min x 522,627 ops/sec ±2.14% (55 runs sampled)
underscore-min is 44% faster.

_.clone with an object:
lodash.min x 72,847 ops/sec ±1.73% (85 runs sampled)
underscore-min x 60,039 ops/sec ±1.91% (76 runs sampled)
lodash.min is 22% faster.

_.compact:
lodash.min x 3,935,837 ops/sec ±1.57% (58 runs sampled)
underscore-min x 1,196,394 ops/sec ±1.15% (60 runs sampled)
lodash.min is 228% faster.

_.compact:
lodash.min x 3,170,202 ops/sec ±0.87% (86 runs sampled)
underscore-min x 391,545 ops/sec ±4.91% (77 runs sampled)
lodash.min is 742% faster.

_.contains iterating an array:
lodash.min x 10,701,042 ops/sec ±1.48% (62 runs sampled)
underscore-min x 2,937,160 ops/sec ±3.31% (61 runs sampled)
lodash.min is 271% faster.

_.contains iterating an array:
lodash.min x 7,973,572 ops/sec ±0.56% (83 runs sampled)
underscore-min x 4,681,045 ops/sec ±1.97% (85 runs sampled)
lodash.min is 73% faster.

_.contains iterating an object:
lodash.min x 566,682 ops/sec ±1.61% (58 runs sampled)
underscore-min x 412,175 ops/sec ±2.82% (55 runs sampled)
lodash.min is 39% faster

_.contains iterating an object:
lodash.min x 286,567 ops/sec ±0.72% (82 runs sampled)
underscore-min x 237,311 ops/sec ±1.02% (86 runs sampled)
lodash.min is 21% faster.

_.defaults:
lodash.min x 291,941 ops/sec ±2.17% (59 runs sampled)
underscore-min x 327,058 ops/sec ±4.09% (56 runs sampled)
underscore-min is 10% faster.

_.defaults:
lodash.min x 64,390 ops/sec ±0.98% (78 runs sampled)
underscore-min x 47,167 ops/sec ±1.31% (82 runs sampled)
lodash.min is 37% faster.

_.each iterating an array:
lodash.min x 1,680,530 ops/sec ±3.36% (43 runs sampled)
underscore-min x 1,211,599 ops/sec ±2.13% (60 runs sampled)
lodash.min is 37% faster.

_.each iterating an array:
lodash.min x 1,332,713 ops/sec ±1.59% (81 runs sampled)
underscore-min x 437,932 ops/sec ±4.86% (73 runs sampled)
lodash.min is 214% faster.

_.each iterating an array with thisArg (slow path):
lodash.min x 230,889 ops/sec ±2.09% (55 runs sampled)
underscore-min x 259,713 ops/sec ±2.65% (57 runs sampled)
underscore-min is 12% faster.

_.each iterating an array with thisArg (slow path):
lodash.min x 145,276 ops/sec ±1.74% (82 runs sampled)
underscore-min x 130,024 ops/sec ±1.30% (84 runs sampled)
lodash.min is 11% faster.

_.each iterating an object:
lodash.min x 462,851 ops/sec ±2.16% (58 runs sampled)
underscore-min x 402,994 ops/sec ±1.73% (59 runs sampled)
lodash.min is 14% faster.

_.each iterating an object:
lodash.min x 265,193 ops/sec ±1.50% (81 runs sampled)
underscore-min x 242,067 ops/sec ±1.58% (81 runs sampled)
lodash.min is 10% faster.

_.extend:
lodash.min x 391,455 ops/sec ±1.74% (60 runs sampled)
underscore-min x 479,454 ops/sec ±4.37% (53 runs sampled)
underscore-min is 19% faster.

_.extend:
lodash.min x 79,184 ops/sec ±1.40% (81 runs sampled)
underscore-min x 57,263 ops/sec ±1.51% (79 runs sampled)
lodash.min is 38% faster.

_.filter iterating an array:
lodash.min x 1,723,711 ops/sec ±1.61% (56 runs sampled)
underscore-min x 1,129,047 ops/sec ±3.54% (57 runs sampled)
lodash.min is 56% faster.

_.filter iterating an array:
lodash.min x 1,106,143 ops/sec ±1.04% (77 runs sampled)
underscore-min x 354,839 ops/sec ±5.26% (71 runs sampled)
lodash.min is 225% faster.

_.filter iterating an array with thisArg (slow path):
lodash.min x 230,036 ops/sec ±1.94% (59 runs sampled)
underscore-min x 239,342 ops/sec ±2.60% (58 runs sampled)
underscore-min is 3% faster.

_.filter iterating an array with thisArg (slow path):
lodash.min x 183,732 ops/sec ±5.04% (78 runs sampled)
underscore-min x 154,306 ops/sec ±4.30% (76 runs sampled)
lodash.min is 18% faster.

_.filter iterating an object:
lodash.min x 365,785 ops/sec ±3.47% (51 runs sampled)
underscore-min x 324,787 ops/sec ±1.42% (58 runs sampled)
lodash.min is 10% faster.

_.filter iterating an object:
lodash.min x 256,340 ops/sec ±0.52% (87 runs sampled)
underscore-min x 220,990 ops/sec ±3.60% (81 runs sampled)
lodash.min is 20% faster.

_.map iterating an array:
lodash.min x 1,719,196 ops/sec ±2.07% (44 runs sampled)
underscore-min x 1,204,887 ops/sec ±3.80% (60 runs sampled)
lodash.min is 45% faster.

_.map iterating an array:
lodash.min x 895,932 ops/sec ±2.39% (72 runs sampled)
underscore-min x 338,448 ops/sec ±3.09% (68 runs sampled)
lodash.min is 167% faster.

_.map with thisArg iterating an array (slow path):
lodash.min x 203,691 ops/sec ±4.32% (55 runs sampled)
underscore-min x 254,401 ops/sec ±2.55% (59 runs sampled)
underscore-min is 27% faster.

_.map with thisArg iterating an array (slow path):
lodash.min x 152,656 ops/sec ±14.98% (70 runs sampled)
underscore-min x 154,615 ops/sec ±2.51% (76 runs sampled)
underscore-min is 14% faster.

_.map iterating an object:
lodash.min x 408,423 ops/sec ±4.37% (55 runs sampled)
underscore-min x 336,210 ops/sec ±2.47% (58 runs sampled)
lodash.min is 19% faster.

_.map iterating an object:
lodash.min x 208,316 ops/sec ±1.72% (75 runs sampled)
underscore-min x 202,225 ops/sec ±1.59% (73 runs sampled)
lodash.min is 3% faster.

_.pluck:
lodash.min x 1,275,845 ops/sec ±4.73% (52 runs sampled)
underscore-min x 858,254 ops/sec ±1.59% (59 runs sampled)
lodash.min is 44% faster.

_.pluck:
lodash.min x 813,716 ops/sec ±3.17% (77 runs sampled)
underscore-min x 304,590 ops/sec ±8.07% (64 runs sampled)
lodash.min is 180% faster.

_.template (slow path):
lodash.min x 12,955 ops/sec ±3.12% (52 runs sampled)
underscore-min x 22,679 ops/sec ±2.75% (54 runs sampled)
underscore-min is 76% faster.

_.template (slow path):
lodash.min x 11,291 ops/sec ±2.32% (71 runs sampled)
underscore-min x 13,216 ops/sec ±1.23% (66 runs sampled)
underscore-min is 18% faster.

Maybe its worth involving the creator of lodash @jdalton in this discussion.

@moll
Copy link
Contributor

moll commented May 7, 2014

Is this even a problem in Keystone that Underscore or Lodash is or isn't fast enough?
Database calls, Mongoose handling, all of the HTTP parsing and stuff probably accounts for 99.(9)% of the request time that the figurative 5 places where Keystone uses _ make no difference at all. Don't waste your time, people. Optimize where it's important.

@kessler
Copy link
Author

kessler commented May 7, 2014

@moll
No body said this is a _problem_ in keystone and I'm not trying to criticize the author's decision to use this or that library in any way. This is merely a naive, simple, _minor_ discussion about optimization and standards that can be pursued in tandem with other efforts of improvements.

For your convenience I've included a regex search for "_." in keystone source files:

docs\build.js (2)
docs\content\pages\docs\database.jade (7)
docs\content\pages\docs\getting-started.jade (1)
docs\docs.js (2)
docs\public\js\lib\bootstrap\transition.js (1)
docs\public\js\lib\jquery\jquery-1.10.2.min.js (1)
index.js (21)
lib\asyncdi.js (5)
lib\content\index.js (2)
lib\content\page.js (1)
lib\content\types\html.js (1)
lib\content\types\text.js (1)
lib\email.js (5)
lib\field.js (8)
lib\fieldTypes\azurefile.js (4)
lib\fieldTypes\boolean.js (1)
lib\fieldTypes\cloudinaryimage.js (3)
lib\fieldTypes\cloudinaryimages.js (3)
lib\fieldTypes\code.js (1)
lib\fieldTypes\date.js (1)
lib\fieldTypes\datetime.js (1)
lib\fieldTypes\email.js (1)
lib\fieldTypes\embedly.js (2)
lib\fieldTypes\html.js (1)
lib\fieldTypes\key.js (1)
lib\fieldTypes\localfile.js (3)
lib\fieldTypes\location.js (12)
lib\fieldTypes\markdown.js (1)
lib\fieldTypes\money.js (1)
lib\fieldTypes\name.js (3)
lib\fieldTypes\number.js (1)
lib\fieldTypes\password.js (2)
lib\fieldTypes\relationship.js (8)
lib\fieldTypes\s3file.js (4)
lib\fieldTypes\select.js (5)
lib\fieldTypes\text.js (1)
lib\fieldTypes\textarea.js (1)
lib\fieldTypes\url.js (1)
lib\list.js (9)
lib\schemaPlugins.js (7)
lib\security.js (3)
lib\session.js (1)
lib\updateHandler.js (4)
lib\updates.js (2)
lib\view.js (2)
public\js\common\ui-cloudinaryimage.js (1)
public\js\common\ui-cloudinaryimages.js (5)
public\js\common\ui-location.js (1)
public\js\common\ui-sortable.js (1)
public\js\common\ui.js (2)
public\js\lib\codemirror\codemirror-compressed.js (257)
public\js\lib\jquery\jquery-1.10.2.min.js (1)
public\js\lib\moment\moment-1.7.2.js (2)
public\js\lib\tinymce\plugins\table\plugin.min.js (34)
public\js\lib\tinymce\tinymce.min.js (24)
public\js\lib\underscore\underscore-1.5.1.js (197)
public\js\lib\underscore\underscore-1.5.1.min.js (1)
public\js\views\item.js (5)
public\js\views\list.js (4)
routes\api\list.js (1)
routes\download\list.js (8)
routes\views\item.js (8)
routes\views\list.js (6)
templates\helpers\emails\mixins\button.jade (4)
templates\mixins\columns.jade (1)
templates\views\list.jade (2)
test\fields.js (4)

If you ignore some of these, like public/js and docs, its still, in my opinion more than "figurative 5 places". Don't let the count confuse you, some of these sites are called in loops many times over and some sit at the very base of the framework.

@jdalton
Copy link

jdalton commented May 7, 2014

I've been sticking to underscore over lodash because they're more strict about only including features that can't be misinterpreted (e.g. the limitations on _.extend) as a language addition, whereas lodash has more features but seems more likely to do something unexpected or non-standard.

Lo-Dash aligns with ES6 Object.assign which is why Lo-Dash's method is called _.assign and is aliased as _.extend. Aligning with ES6 early gives Lo-Dash the option to use native if it's faster without worrying about breaking back-compat.

A big reason I created Lo-Dash was to address cross-environment consistency issues that Underscore refused to tackle. To further reduce the unexpected, Lo-Dash also follows semver, so unlike Underscore we don't introduce back-compat breaking changes with each minor release.

I'm happy to go with community consensus on this and switch. @kessler could you be a bit more specific though with your "far superior in many ways" comment?

It comes down to consistency, support, features, customization, & performance. Lo-Dash goes above and beyond to ensure consistent behavior of its methods in all supported environments. We've listened to the community and added features like a deep clone and merge, that time after time have been punted on by Underscore. Lo-Dash also gives you more ways to consume the utility methods even as bundles of AMD modules, Node modules, or individual npm packages.

The speed improvement seems like the biggest argument (given that common niceties we need are also added to the keystone-utils library as required). I've cherry-picked the benchmarks that are relevant based on function I know keystone uses often, and excluding ones too close to call:

Performance is great and there are lots-of-examples of devs seeing performance improvements by switching, but performance in Lo-Dash has always taken a back seat to consistency and correctness. I used performance to offset the cost of fixes/consistency and then took a step back and realized it went a bit further ;D

Finally, it may also be worth switching to use of native methods as a rule for functionality available in the V8 implementation of JavaScript (e.g. Array.prototype.filter rather than _.filter) for the biggest speed improvement; to date I've generally been using _ so there's less context switch between writing client and server side code.

Actually native Array#filter and friends are historically slow. In its next release Underscore is aligning with Lo-Dash and ditching ES5 array method use in favor of simple for-loops. This offers performance, consistency, & added features over native alternatives.

It's a bit hard to explain what I mean by that - generally underscore seems more inline with how features (_.each, _.map, etc) are eventually implemented in JavaScript.

Both Lo-Dash and Underscore stray from the spec'ed native methods in ways, usually to be more usable or add popular features. The difference is Lo-Dash is consistent where Underscore is not. For example Underscore will defer to native if it exists but fallback to its own implementation if it doesn't. The problem is its fallbacks don't align with native so depending on your environment the methods will act differently and produce different results. Underscore is taking steps to address this in their next bump by aligning closer to Lo-Dash.

Underscore has started aligning its API to Lo-Dash. For example they added chainable _.each, AMD support, & bower/component support in Underscore v1.6.0, and have in flight support for iterator shorthands, iterator support for _.omit and _.pick, exposed memoization cache, more consistent handling of nullish args, and more changes aligning with the behavior established by Lo-Dash.

@moll
Copy link
Contributor

moll commented May 7, 2014

Lo-Dash aligns with ES6 Object.assign which is why Lo-Dash's method is called _.assign and is aliased as _.extend. Aligning with ES6 early gives Lo-Dash the option to use native if it's faster without worrying about breaking back-compat.

The gotcha about _.extend iterating over own properties as opposed to all extends farther than _.assign: it's also the basis for _.defaults, _.map, _.isEmpty and others. I'm not sure if this is the place to discuss the shortcomings of that design, but I definitely would not take this lightly. It brings a fair amount of risk and idiosyncratic behavior to the code. Inherited properties are a core (and underused, I'd say) part of prototypical languages and having 3rd party libraries, which take advantage of either Underscore or Lodash, behave so differently and surprisingly depending on which of those _'s the author chose, is a yet another nail in the programming sucks coffin. It's like the Ruby problem of mixing symbols and strings and then spending hours trying to debug why the hell do some properties work and some not.

And I would bet a few of my cookies that most authors are fully ignorant of this tradeoff.

@moll
Copy link
Contributor

moll commented May 7, 2014

Anyone wanna bet that there's a security problem lurking in some app or library purely from the behavior of Lodash's _.isEmpty? ;)

@jdalton
Copy link

jdalton commented May 7, 2014

The gotcha about _.extend iterating over own properties as opposed to all extends farther than _.assign: it's also the basis for _.defaults, _.map, _.isEmpty and others.

It does have some Underscore-compat side effects but they are minimal and have been worked around with methods like _.create instead.

The methods you listed, _.map and _.isEmpty, iterate over own-properties in both Underscore and Lo-Dash. However, unlike Underscore, Lo-Dash gives devs the ability to explicitly iterate over own object properties, via _.forOwn, or both (own+inherited), via _.forIn.

It brings a fair amount of risk and idiosyncratic behavior to the code.

You can always use the lodash.underscore build to ease in to the switch.

Inherited properties are a core (and underused, I'd say) part of prototypical languages

I get that which is why we follow the language precedents and offer things like _.create as our Object.create as it's specifically for prototypal inheritance. Underscore is more wish-washy with its method behavior which creates some interesting oddities. For example because their _.extend method iterates over inherited properties their shallow clone will create a new object with own properties consisting of the inherited and own properties of the source object.

@jdalton
Copy link

jdalton commented May 7, 2014

Anyone wanna bet that there's a security problem lurking in some app or library purely from the behavior of Lodash's _.isEmpty? ;)

Speaking of QA, Underscore doesn't even run its unit tests in Node (npm test uses PhantomJS).
On the other hand Lo-Dash has ~100 code coverage and unit tests which run in node, ringo, rhino, narwhal, phantomjs, and browsers for various builds, minified and not, per check-in.

@moll
Copy link
Contributor

moll commented May 7, 2014

The methods you listed, _.map and _.isEmpty, iterate over own-properties in both Underscore and Lo-Dash. However, unlike Underscore, Lo-Dash gives devs the ability to explicitly iterate over own object properties, via _.forOwn or both, via _.forIn.

Ah, shit, you're right @ Underscore.isEmpty. Great, inconsistency rocks indeed!

Well, yeah, Lodash.forIn being there is great, but a for in loop isn't the annoying part about JavaScript. It's the object merging functions like _.extend, _.defaults and such that are most useful and I'd bet the most used parts. And that's, I'd argue, where Lodash's behavior will hurt everyone in the long run:

var opts = Object.create(myDefaults)
opts.foo = 42
yourFunction(opts)

function yourFunction(opts) {
  opts = _.extend({}, yourDefaults, opts)
  if (opts.foo != null) somethingOrOther()
}

Remove the yourDefaults line and you've got the code running somethingOrOther. Add it and it won't.

The thing is, if you find inherited props to be a core part as well, then you should see that Lodash's behavior is actively harmful for the use of inherited properties beyond classes. The worse part, and that's not entirely your fault (:-)), is that the support or non-support of inherited properties will become a hidden API detail — an implementation detail leaking to the public API.

@jdalton
Copy link

jdalton commented May 7, 2014

It's the object merging functions like _.extend, _.defaults and such that are most useful and I'd bet the most used parts. And that's, I'd argue, where Lo-Dash's behavior will hurt everyone in the long run

Lo-Dash's behavior aligns with the language and has been a non-issue IRL. If you want inheritance, flattening it onto an object isn't the way to go about it (that's not inheritance). You should use _.create instead.

The thing is, if you find inherited props to be a core part as well, then you should see that Lo-Dash's behavior is actively harmful for the use of inherited properties beyond classes.

Keeping a clear documented distinction and aligning with language precedents is the way to go. Being ambiguous is the real danger which leads to real API issues (as I mentioned previously).

@moll
Copy link
Contributor

moll commented May 7, 2014

Lo-Dash's behavior aligns with the language and has been a non-issue IRL.

Would you mind clarifying which parts of the language do you mean with aligning? Object.assign is just one part of a future language. Wouldn't you say property access and a for in loop are more a language than a single stdlib function is?

If you want inheritance, flattening it onto an object isn't the way to go about it (that's not inheritance). You should use _.create instead.

Well, I'd say inheritance must be an implementation detail to the receiver — isn't that how inheritance and polymorphism are supposed to work? In the example, yourFunction just wants to get a single options object with its own defaults put it (obviously it mustn't change the given object).

Also, think of getters: You have an object with a getter set in the prototype that will return something. Pass that object to _.extend and that property will be ignored. Use for in to manually "extend" an object and it'll appear. Not inconsistent with the language? ;-)

@jdalton
Copy link

jdalton commented May 7, 2014

Would you mind clarifying which parts of the language do you mean with aligning? Object.assign is just one part of a future language.

Yes, Object.assign is one part, but it's the part that corresponds to this API and something devs will be using as an alternative to $.extend and _.extend. Vendors are already implementing parts of ES6. In fact, Lo-Dash (edge) uses ES6 Set to optimize large array operations.

Wouldn't you say property access and a for in loop are more a language than a single stdlib function is?

I'd say that a object iteration, via a for-in loop, is part of it too. We provide _.forIn because iterating objects via a for-in loop is inconsistent across environments.

Well, I'd say inheritance must be an implementation detail to the receiver — isn't that how inheritance and polymorphism are supposed to work?

Ya well, that's like your opinion man, (big lebowski). The common case is iterating over own properties. This allows optimizations like using Object.keys under the hood too. By the way, Node aligns with Lo-Dash in its util._extend with respect to own-property iteration as well.

(obviously it mustn't change the given object)

That's right and I pushed Underscore to fix that in their implementation of methods like _.template so they wouldn't augment user provided options objects.

Also, think of getters: You have an object with a getter set in the prototype that will return something.

Getters/Setters are out of scope for both Lo-Dash and Underscore as they can't be addressed consistently across environments. For example neither handle getters/setters in _.clone or _.isEqual.

@moll
Copy link
Contributor

moll commented May 7, 2014

The common case is iterating over own properties.

What is the use case that supposedly turns iterating over own properties to the common case? People are JSON.parseing and extending defaults options left and right, for sure, but I sincerely doubt most of them use _ functions on instances of classes with functions in the prototype. If they do, it's to extend that particular class, not extend an object with that class. Backbone, one of the most common model libraries for JavaScript, specifically avoids this entire problem by using an attributes property.

Yes, Object.assign is one part, but it's the part that corresponds to this API and something devs will be using as an alternative to $.extend and _.extend.
[..]
This allows optimizations like using Object.keys under the hood too. By the way, Node aligns with Lo-Dash in its util._extend with respect to own-property iteration as well.

I'm sure you noticed, but util._extend is a private method and should not enter any discussions.
As for jQuery, I don't know which version you're checking, but at least v1.9's $.extend does seem to iterate over inherited properties. Object.assign won't fit in for $.

Don't get me wrong, my arguments are not only in regards to _.extend (keeping _.assign out of the question for sec). They're against the concept of not taking inheritance into account which most other functions in Lodash follow as well.

I'd say that a object iteration, via a for-in loop, is part of it too. We provide _.forIn because iterating objects via a for-in loop is inconsistent across environments.

Yet Lodash doesn't provide a function to extend objects with inherited properties or do any of the other useful things it does.

Getters/Setters are out of scope for both Lo-Dash and Underscore as they can't be addressed consistently across environments. For example neither handle getters/setters in _.clone or _.isEqual.

Oh, I didn't mean that. Especially because the use of getters must be an implementation detail that must not change behavior. I meant if I give you an object (an options object, e.g.) with a getter your function should behave equivalent regardless if the getter was in the prototype or as an own property. Lack of setters is not a problem, as we just agreed that passed-in objects should never be changed.

@jdalton
Copy link

jdalton commented May 7, 2014

What is the use case that supposedly turns iterating over own properties to the common case?

It's the common case in both Underscore and Lo-Dash.

People are JSON.parseing and extending defaults options left and right, for sure, but I sincerely doubt most of them use _ functions on instances of classes with functions in the prototype.

I'm sure devs do use Lo-Dash for OOP. We've fixed object iteration bugs to aid in that.

I'm sure you noticed, but util._extend is a private method and should not enter any discussions.

Sure it's pseudo private, but it's being used by Node to do things like applying options objects which makes it very relevant to this discussion.

As for jQuery, I don't know which version you're checking, but at least v1.9's $.extend does seem to iterate over inherited properties.

Yap, jQuery doesn't currently align with Object.assign and may not be able to because they have a strong back compat story. However, that didn't stop Rick Waldron, a jQuery developer and TC39 representative, from ironing out the implementation of Object.assign. Many devs use Underscore or Lo-Dash mixed with jQuery without issues.

Yet Lo-Dash doesn't provide a function to extend objects with inherited properties or do any of the other useful things it does.

Lo-Dash and Underscore are about providing the small bits devs can use to create other things. So Lo-Dash offers _.forIn and _.forOwn so devs can get consistent object iteration and then, in turn, create their own _.mixins.

I meant if I give you an object (an options object, e.g.) with a getter your function should behave equivalent regardless if the getter was in the prototype or as an own property.

Getters can have side effects too, neither libs support them.

You like Underscore? Great, use the lodash.underscore.js build. You'll get drop-in Underscore compatibility plus a bit of perf & consistency to boot.

@kessler
Copy link
Author

kessler commented May 8, 2014

@jdalton thank you very much for your extended input on this issue, it has been educating.
@moll I hope your "curiosity" was satisfied?

@JedWatson
Copy link
Member

Thanks both @jdalton and @moll for some really insightful discussion - when I pushed back on the simplicity of this PR's original description for more detailed reasoning, let's just say this response really blows my expectations out of the water.

I also really appreciate your weigh-in, @jdalton, it's great to have the differences, background and reasoning explained. I'm now very comfortable to switch to Lo-Dash for keystone and keystone-utils, and appreciate the benefits.

As @moll has explained it's not a perfect environment, there are a number of inconsistencies and in this case we've got the advantage that this decision is just for keystone itself (and will only be run in node) and project authors can still use whichever library they prefer.

Regarding the difference in behaviours of _.extend, as far as I'm aware there's no code in Keystone itself that uses this on objects that would have inherited properties - it's more used for defaulting options arguments and similar operations, so this shouldn't cause a problem.

Because jQuery came up it might be worth mentioning that I generally use jQuery and underscore Lo-Dash ;-) together for client-side code, using jQuery for its DOM functionality but _ for language-level features and polyfills, because (in my experience / opinion) jQuery is more likely to conflict with standards (e.g. jQuery.forEach(collection, iterator(index, value)) as opposed to _.each(collection, iterator(value, index))). So there's a clear separation of usage in my mind despite some feature crossover. Would be interested if anyone has other views on this.

Actually native Array#filter and friends are historically slow. In its next release Underscore is aligning with Lo-Dash and ditching ES5 array method use in favor of simple for-loops. This offers performance, consistency, & added features over native alternatives.

This is really good to know, and a bit surprising. I'll continue preferring _ for supported features over native implementations, which is nice (as I mentioned) for consistency between client and server side code.

@JedWatson
Copy link
Member

@kessler would you mind rebasing this PR so I can merge it without conflicts? looks like there's been a change github can't automatically merge. otherwise I'll come back to it when I've wrapped up some other changes I'm working on.

@kessler
Copy link
Author

kessler commented May 8, 2014

@JedWatson no probs, will do.

@moll
Copy link
Contributor

moll commented May 8, 2014

@jdalton:

Sure it's pseudo private, but it's being used by Node to do things like applying options objects which makes it very relevant to this discussion.

You're right. I've submitted a bug to Node.js for that. ;-)

Indeed, thanks @jdalton for the discussion! I would still argue that Lodash's design and behavior around inherited properties is to the detriment of the whole language and simplicity by adding to the inconsistency of APIs, but hey, we can find another place to continue or just create our own Underscores. ;-)

@JedWatson:

Regarding the difference in behaviours of _.extend, as far as I'm aware there's no code in Keystone itself that uses this on objects that would have inherited properties - it's more used for defaulting options arguments and similar operations, so this shouldn't cause a problem.

Unfortunately that's the place where it matters. If you use _.defaults, with Lodash, your API states it will ignore inherited properties. But be sure to document this and be consistent — that means never using if (options.foo) directly as well without removing the inherited properties otherwise you will take inherited properties into account in some places. ;-)

@JedWatson:

This is really good to know, and a bit surprising. I'll continue preferring _ for supported features over native implementations, which is nice (as I mentioned) for consistency between client and server side code.

In a controlled environment like Node.js on V8 I doubt that's going to be an issue. Keeping 3rd party dependencies low has its own benefit and the Node stdlib itself seems to find plain JavaScript totally suitable as it doesn't use _ internally. :-)

@moll
Copy link
Contributor

moll commented May 8, 2014

@kessler would you mind rebasing this PR so I can merge it without conflicts? looks like there's been a change github can't automatically merge. otherwise I'll come back to it when I've wrapped up some other changes I'm working on.

You could also use the lodash-node module like @jdalton suggested and use the Underscore compatible build there to get the good-enoughs of both worlds. Would save you having to inspect the hundred places where inherited vs own properties will end up creating inconsistencies.

@jdalton
Copy link

jdalton commented May 8, 2014

@moll

I would still argue that Lo-Dash's design and behavior around inherited properties is to the detriment of the whole language and simplicity by adding to the inconsistency of APIs, but hey, we can find another place to continue or just create our own Underscores. ;-)

I'm sorry, I don't agree. Your doomsday stance is totally unfounded and a bit over the top. I've shown that your concern is not a real world issue. Lo-Dash has more than 2 million npm downloads a month and more than 3,000 dependents on npm alone. No one is hung up on this _.extend behavior. The fact that KeystoneJS doesn't have an issue and that Node aligns with Lo-Dash, and has for years, is even more evidence of that.

— that means never using if (options.foo) directly as well without removing the inherited properties otherwise you will take inherited properties into account in some places. ;-)

Again, more of the same.

Keeping 3rd party dependencies low has its own benefit and the Node stdlib itself seems to find plain JavaScript totally suitable as it doesn't use _ internally. :-)

Node may not use _ but they have lots of utils . Having a lib layer on top enables devs to write less code and provides wiggle room in terms of performance and refactoring.

You could also use the lodash-node module

You could also do require('lodash/dist/lodash.underscore') if needed.

and use the Underscore compatible build there to get the good-enoughs of both worlds.

The Underscore compat build is not equiv to Lo-Dash. In order to be a drop-in replacement all of Lo-Dash's features are removed. Using the Underscore compat build is a good first step but I'd suggest trying the full Lo-Dash first to see if there's even an issue.

@moll
Copy link
Contributor

moll commented May 8, 2014

I'm sorry, I don't agree. Your doomsday stance is totally unfounded and a bit over the top. I've shown that your concern is not a real world issue. Lo-Dash has more than 2 million npm downloads a month and more than 3,000 dependents on npm alone. No one is hung up on this _.extend behavior. The fact that Node itself aligns with Lo-Dash is even more evidence of that.

Oh, absolutely, things are not going to blow up all at once. It's a gradual decline as with any complexity: things go under the radar until it's a big ball of mud, and by then, everybody's turned to pigs and love rolling around. I wish I could join, but I've got these new blue suede shoes I'd love to keep clean. :)

Popularity, however, should never be confused with correctness or simplicity. API design has many parallels with the human-computer interaction field and you don't see anyone there suggesting waiting until >50% of the people complain to do anything about it.

The Underscore compat build is not equiv to Lo-Dash. In order to be a drop-in replacement all of o-Dash's features are removed.

Oh, though when I checked out lodash-node/underscore, I saw references to _.partialRight and other Lodash-only functions. Did I read that wrong?

@jdalton
Copy link

jdalton commented May 8, 2014

Oh, absolutely, things are not going to blow up all at once. It's a gradual decline as with any complexity:

Again, I don't agree.

everybody's turned to pigs and love rolling around

Should I take that as a personal attack or an attack against Lo-Dash? 😀

Popularity, however, should never be confused with correctness or simplicity. API design has many parallels with the human-computer interaction field and you don't see anyone there suggesting waiting until >50% of the people complain to do anything about it.

Popularity helps put into context the issue and gives us the reach to be exposed to issues big and small. If it was a pressing problem we would have addressed it.

Oh, though when I checked out lodash-node/underscore, I saw references to _.partialRight and other Lo-Dash-only functions. Did I read that wrong?

Yah, you read that wrong. The _.partialRight method is not included in the Node module bundles for the Underscore build. See https://github.com/lodash/lodash-node/tree/2.4.1/underscore

@moll I think you've derailed the thread enough.
We'll continue the inherited property discussion over on the Node thread.

@moll
Copy link
Contributor

moll commented May 8, 2014

Should I take that as a personal attack or an attack against Lo-Dash? 😀

What do you have against pigs? They're so cute! 🐷

@moll I think you've derailed the thread enough. We'll continue the inherited property discussion over on the Node thread.

I'd say our discussion was rather relevant to the whole Underscore vs Lodash topic, but I'll see you at the Node thread!

@kessler
Copy link
Author

kessler commented May 9, 2014

@moll - that after all this fuss, will you help me make the transition of keystone to lodash a safe one?

@moll
Copy link
Contributor

moll commented May 9, 2014

@kessler: How may I do so?

A side question — is @JedWatson the benevolent dictator in Keystone's case or who's the final decision maker?

@kessler
Copy link
Author

kessler commented May 9, 2014

@moll obviously you have a much deeper understanding than me about both keystone and the fine prints of inherited properties, so it would be very helpful if you can simply go over the diff.

As for the dictatorship status I have no clue, but hopefully @JedWatson could shed some light over this.

@moll
Copy link
Contributor

moll commented May 9, 2014

Oh, thanks, I'm flattered, but unfortunately I do not have enough knowledge of Keystone's internals to know which public APIs will be affected by the inherited-options issue. I'm confident @JedWatson will be able to tell in a heartbeat. You could also use, like John-David suggested, the lodash/dist/lodash.underscore file to get the exact Underscore's behavior.

Whichever way @JedWatson decides to go, I would support consistency and clear documentation. If inherited properties are to be disallowed, they should be ignored in every API and documented clearly in the API docs.

@JedWatson
Copy link
Member

I've been looking into it further and think the best option is to delay this for a little bit. There are quite a few changes going on in master right now and a few things that could be affected by it.

I would like to switch to LoDash in the near future, for performance and other benefits, but for consistency we should switch the core keystone package, keystone-utils and probably the front-end stuff over all at once.

I'd also like to (a) make it an isolated change that we can test for a bit, and (b) have some more automated tests in place first so we can make the switch with better confidence.

Thanks to @kessler for bringing this up in the first place, and to both @jdalton and @moll for your input and caring so much about the issues involved. I really appreciate it.

Oh, and Keystone's leadership hasn't come up before but yes, it would be me.

@JedWatson JedWatson closed this May 10, 2014
@sebmck sebmck mentioned this pull request Jan 24, 2015
duidae added a commit to duidae/keystone that referenced this pull request May 8, 2022
duidae added a commit to duidae/keystone that referenced this pull request May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants